Site Editor: Merge the post only mode and the post editor #56671
Conversation
youknowriad
added
[Type] Enhancement
youknowriad
requested review from
ajitbohra,
ryanwelcher,
juanmaguitar,
fabiankaegy and
ndiego
as code owners
youknowriad
requested review from
mcsf,
ellatrix,
ramonjd,
tellthemachines and
andrewserong
| return false; | ||
| } | ||
|
|
||
| export default function EditorCanvas( { |
There was a problem hiding this comment.
This has been mostly copied from the "VisualEditor" component of edit-post. The ultimate goal is to replace it entirely and also replace "SiteEditorCanvas" from edit-site but there are more steps to be done before being able to do that. For now, it only renders the "BlockList" basically and the "PostTitle" component if needed.
|
Size Change: +1.75 kB (0%) Total Size: 1.72 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in bafdc95. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/7044584156
|
| <> | ||
| <LayoutStyle | ||
| selector=".editor-editor-canvas__post-title-wrapper" | ||
| layout={ fallbackLayout } | ||
| /> | ||
| <LayoutStyle | ||
| selector=".block-editor-block-list__layout.is-root-container" | ||
| layout={ postEditorLayout } | ||
| /> |
There was a problem hiding this comment.
One thing I'm wondering is whether it makes sense to use two different layouts for "post-content" and "post-title". It seems like "post-title" here is always using the default layout from theme.json while for "post-content" we try to guess the layout from the block. I'm wondering whether that makes sense, if these two things are different (which can be in some cases), it would result in some weird alignment in the post editor.
I wonder if we should always use the "post content" one for both.
There was a problem hiding this comment.
Post Title doesn't have layout itself, so we can't guess at its exact placement on the page. We did at one point try using Post Content layout for it, but that caused problems as reported in #48579, so it was switched back.
There was a problem hiding this comment.
I mean that issue doesn't really say why it was "bad". For me it seems just like a different random choice.
There was a problem hiding this comment.
It didn't seem to be intentional, just a side-effect of #46698, which was only trying to add padding to the title. Yes it's random in the sense that there are no documented decisions around how it should work, so changes have mostly been made reactively based on feedback.
I'm not saying Post Title absolutely shouldn't have the same layout as Post Content, but if it does we should be aware of all the possible implications and probably note somewhere that it's a deliberate design decision.
|
In my testing I found two issues. I don't know if they are specific to this PR, but I had never seen them before:
testing-merge-canvases-site-and-post.mov |
|
Hey @youknowriad 👋 Thanks for working on this! I'm wondering whether you could clear up some things for me :)
|
Yes, no impact, in fact I don't think this PR impacts the post editor at all (or at least it shouldn't unless I did some mistake)
Preview option is actually already in the post editor (different UI slightly, you click the template on the sidebar and click "edit template"), but yes, my goal is to unify that mode with the site editor. Maybe the starting point will be different between post and site editors but the capabilities and the code base will be exactly the same. |
|
Working nicely for me. 👏🏻
One thing about the site editor Template preview mode:
Trunk2023-12-01.12.25.33.mp4This PR2023-12-01.12.25.52.mp4Following on from this discussion: #56000 (comment) Would this be classed as a regression? Just checking that it's intended to change the way the page's block order is rendered in the site editor. I know the goal is to unify components, but it might be good to get UX/design input to confirm if it departs from #41717 For the record, I'm fine with either. A follow up to this PR however might be to reconsider how we load pages in the site editor so that there difference in functionality between it and the post editor is more discernible , e.g., load the page in its template first, then the toggle could be "Edit page" instead of "Template preview". Not sure 🤷🏻 |
100% - and I appreciate the huge effort to consolidate this stuff. 🙇🏻 I think what I was rambling about above what more about the messaging. I think it's just "Template preview" that's catching me off guard. 😄 If we're going to remove the guesswork of 1:1 post and template content in the site editor, then maybe the site editor could have the same UI as the post editor, but with "Edit page". 
cc @SaxonF |
|
Thanks for persisting across all these different PRs, it does feel like we're inching toward a better solution all around!
Good point, it is really nice being able able to hit enter to move from the post title to the post content. I also like that while editing in the post only mode, the list view displays the blocks within post content at the root level rather than being nested within a "content" block 👍 Styling-wise, the main issue I ran into so far is that the root padding rules don't appear to be output within the site editor's post only mode, this means text bumps up against the edges of the editor canvas. In the below screengrab, on the left is the site editor with template preview switched off, and on the right is the page editor with the same page (running TwentyTwentyFour theme):
Another visual difference is that in the page editor, if the template sets a custom content/wide size, that's respected in the editor, but that doesn't appear to be the case in the site editor's post only mode. Below, the left is the site editor, the right is the page editor:
I can give the PR some more detailed testing next week! |
It is definitely my goal to have the same UI but we can't have the same UI before having the same code controlling it because the UI is just going to change "mode". I don't want to duplicate the UI component.
I'm really surprised about this, I'll debug. |
|
@mcsf I'm not able to reproduce any of your issues. Can you give more instructions about the theme, the page you're editing... |
|
@andrewserong The issue was that neither |
There was a problem hiding this comment.
Thanks @youknowriad, the styling is all looking good to me now!
✅ Layout from the post content block is now reflected properly when template preview is switched off, along with padding rules
✅ Tested that the post and page editors still reflect server-loaded post content attributes, and work with custom layout content sizes when editing posts and pages via an Author role
This PR's looking in quite good shape. A couple of other minor things I noticed in re-testing:
- When template preview mode is toggled off, and the list view now lists everything within post content at the root level (just like in the post editor), the Content panel on the right hand side is a bit noisy, as it too lists everything. I think that panel was only meant to be there so that folks could easily click between the content blocks (e.g. featured image, title, content), so should we hide this if template preview mode is disabled?
- When template preview mode is toggled off, there is no way to update the featured image for the page. I imagine this is likely not a blocker, and given the discussion surrounding further consolidation in follow-ups, perhaps we could pull over the featured image UI from the post / page editor if need be in a follow-up?
We might want to confirm with @mcsf if the issues encountered during testing can be reproduced still? I wasn't able to reproduce those locally in TT4, but could have missed something.
Otherwise, this is all looking good pretty good to me! Just left a question about the classname change for the post title, wrapper, too, as I'm wondering if that'll be a breaking change for some themes 🤔
| { renderingMode === 'post-only' && ( | ||
| <div | ||
| className={ classnames( | ||
| 'editor-editor-canvas__post-title-wrapper', |
There was a problem hiding this comment.
Just thinking: is this a potentially breaking change? I'm wondering if Classic themes or other themes in general have been using the existing classname for different purposes. There seems to be a few results searching for this classname across the theme directory: https://wpdirectory.net/search/01HGSH721NFYVA5XKZ4KAGYCT7
Clicking through from there, I can see that Blocksy theme for example seems to set the post title font size based on the wrapper classname 🤔
Would it be worth making the classname here configurable? I.e. could the instance of <EditorCanvas> used in the post editor pass down a classname to use for backcompat?
There was a problem hiding this comment.
So we state in our docs and guidelines that "classnames" for components are not public APIs and that people shouldn't rely on them too much. That said, for this particular case, it's a bit sensitive so I agree with you.
I don't think making the classname configurable is worth it but good point, I'll keep the old class name to avoid breaking changes.
I think it makes sense to hide that panel yeah in that mode yes.
yes, my goal is to have the exact same UI for both post and site editor, sidebar, top bar, everything. So yeah, this is going to be one of the follow-ups. |
|
This should be ready for ✅ |
I was on standard TT4 and just editing a custom page using the site editor's navigation. Maybe it was a fluke. |
There was a problem hiding this comment.
Thanks for all the follow-ups @youknowriad!
This is testing well for me, tested with a variety of blocks and classic themes, and I didn't run into any blockers.
Left a tiny note about a difference in top margin between em and rem units, but from my perspective, this PR is in good shape now, and I think it'll be easier to do subsequent tweaks in follow-ups.
Great work again on all the consolidation efforts, there's a lot of complexity to untangle in all of this, and with this work it feels like we're heading in a better direction code-wise, which will improve and unlock further progress 👍
LGTM! ✨
| <div | ||
| className={ classnames( | ||
| 'editor-editor-canvas__post-title-wrapper', | ||
| // The following class is only hear for backward comapatibility |
There was a problem hiding this comment.
Tiniest of nits 🙂
| // The following class is only hear for backward comapatibility | |
| // The following class is only here for backward comapatibility |
| style={ { | ||
| // This is using inline styles | ||
| // so it's applied for both iframed and non iframed editors. | ||
| marginTop: '4em', |
There was a problem hiding this comment.
In the post editor, on trunk this is 4rem instead of 4em, so in testing manually there is a slight yet subtle difference. Not a blocker at all, just thought I'd mention it in case it's something you wanted to tweak in a follow-up (I'm not sure which one's preferred).
There was a problem hiding this comment.
If I had to guess, I'd say rem is preferred because it's relative to :root and not influenced by the element's parent(s)? Normally it scales more uniformly with rem units.
Related #52632
Alternative to #56542 and #56586 and #56631
What?
This PR merges the "canvas" of both the site editor and post editor under a unique component, and also makes the "post-only" mode of the site editor strictly equivalent to the default mode of the post editor as well.
Only potential downside of this PR is that we had some logic in the "post-only" mode of the site editor where we try to guess which "post content block" to render based on the template. This PR removes that and just applies the post editor logic (render post title and post content). I think the advantages of this PR greatly outweigh the potential downsides of this removal. (see the three linked alternative PRs that tried to do the same while keeping the template logic) .
For example this PR brings some niceties of the post editor to the site editor post only mode like hitting "Enter" from the post title to go to the post content.
Testing Instructions
1- Test the post editor: creating, saving posts, switching to template mode...
2- Test the site editor: especially the "post-only" mode (disabling template preview)